apply additional command overrides based on report format #381
Conversation
Codecov Report
@@ Coverage Diff @@
## master #381 +/- ##
=======================================
Coverage 67.08% 67.08%
=======================================
Files 36 36
Lines 7532 7532
=======================================
Hits 5053 5053
Misses 2078 2078
Partials 401 401
Continue to review full report at Codecov.
|
internal/driver/driver.go
Outdated
| @@ -149,7 +151,7 @@ func generateReport(p *profile.Profile, cmd []string, vars variables, o *plugin. | |||
| return out.Close() | |||
| } | |||
|
|
|||
| func applyCommandOverrides(cmd []string, v variables) variables { | |||
| func applyCommandOverrides(cmd []string, outputFormat int, v variables) variables { | |||
There was a problem hiding this comment.
How about changing the first argument to be just "cmd string"? It doesn't seem to be necessary to pass the array.
internal/driver/driver.go
Outdated
| @@ -176,6 +178,12 @@ func applyCommandOverrides(cmd []string, v variables) variables { | |||
| v.set("nodecount", "80") | |||
| } | |||
| } | |||
|
|
|||
| if outputFormat == report.Proto { | |||
There was a problem hiding this comment.
Why did you keep case "proto", "raw": in the switch, isn't it redundant now?
There was a problem hiding this comment.
It's now redundant. Done.
internal/driver/driver.go
Outdated
| @@ -176,6 +178,12 @@ func applyCommandOverrides(cmd []string, v variables) variables { | |||
| v.set("nodecount", "80") | |||
| } | |||
| } | |||
|
|
|||
| if outputFormat == report.Proto { | |||
There was a problem hiding this comment.
Side question: why does "peek" case resets the filter but not not the tagfilter?
There was a problem hiding this comment.
It does seem odd. My best guess is that tagfilter was added later, and this is an oversight, but I'll look into the commit history to see if there's evidence for that.
There was a problem hiding this comment.
Tag filtering was added after the peek command, (in this PR), which suggests to me this is an oversight.
I've changed it in this PR. Let me know if you think it ought to be a separate PR.
internal/driver/driver.go
Outdated
| @@ -176,6 +178,12 @@ func applyCommandOverrides(cmd []string, v variables) variables { | |||
| v.set("nodecount", "80") | |||
| } | |||
| } | |||
|
|
|||
| if outputFormat == report.Proto { | |||
| trim, tagfilter, filter = false, false, false | |||
There was a problem hiding this comment.
Perhaps for future unless there is an easy way to do it: it would be good to have a way to generate reports in the proto format keeping the filter. E.g. using the internal -flame command with a -tagfocus filter to upload a subset of the profile.
There was a problem hiding this comment.
I'm assuming that this could be done just by not setting filter and tagfilter to false. Not sure if we would want to also, for example, add a comment to the profile to indicate filtering was done.
Will experiment, then look into changing.
|
PTAL |
|
@nolanmar511 Appveyor build failed? |
|
Appveyor has an 503 error when installing graphviz with chocolatey. I've retried and am still getting this error. Not sure what's up, though I thinking waiting a bit could help (but that's maybe not the best solution). |
|
@nolanmar511 It would be good to comment on the PR before merging about how the Appveyor failure was fixed - did you do something? did it just go away? something else? |
* 'master' of github.com:google/pprof: (32 commits) record diff base profile label key/value constant (google#384) apply additional command overrides based on report format (google#381) profile: fix legacy format Go heap profile parsing (google#382) add -diff flag for better profile comparision (google#369) Use -u flag in pprof installation command so that it updates if needed. (google#376) Add GetBase support for ASLR kernel mappings (google#371) Add show_from profile filter. (google#372) Update README.md due to 8dff45d (google#375) Remove stale docs, move useful ones. (google#374) internal/driver: skip tests requiring tcp on js (google#373) Add "trim path" option which can be used to relocate sources. (google#366) Move update_d3flamegraph.sh script from the root directory. (google#370) Skip unsymbolizable mapping during symbolz pass. (google#368) remove -positive_percentages flag (google#365) Render icicle graph in "Flame Graph" view (google#367) Add command-line editing support for interactive pprof (google#362) Improve profile filter unit tests, fix show filtering of mappings (google#354) Fake mappings should be merged into a single one during merging (google#357) Hack the code so that both existing Go versions and current Go master format it the same (google#358) moved filter tests into their own test file which matches the implementation file, filter.go. (google#356) ...
* apply additional command overrides based on report format * address comments * update filtering
No description provided.